resolve method can use dependency injection #520

crissi · 2019-11-10T20:56:03Z

the resolve method now only have 3 params that are required
any class can be injected in the resolve method, including the ResolveInfo and SelectFields class
you no longer need to use $selectfields as a closure, but is still supported for when you want to change the depth

*make resolve works more like the controller methods

mfn · 2019-11-11T07:43:59Z

Just a few high level feelings, will do a detailed review at a later time:

I like the concept
I wish, in this case, the container would be already "there" (app()…) because I also foresee use cases within other resolvers having access to it directly)=
I'm worried about performance impact (reflection…) and the additional logic I see
I'm worried about correctness with (apollo) batched requests

mfn · 2019-11-12T22:11:15Z

I'm worried about performance impact (reflection…) and the additional logic I see

I picked the test \Rebing\GraphQL\Tests\Database\SelectFieldsTest::testWithNonNullSelectFieldsAndModel

changed the call to this

      foreach (range(1, 10000) as $_) {
          $response = $this->call('GET', '/graphql', [
              'query' => $graphql,
          ]);
      }

ran it 3 times with master and with this PR

master:

Time: 15.07 seconds, Memory: 30.00 MB
Time: 14.39 seconds, Memory: 30.00 MB
Time: 14.49 seconds, Memory: 30.00 MB

This PR:

Time: 15.37 seconds, Memory: 30.00 MB
Time: 17.97 seconds, Memory: 30.00 MB
Time: 15.63 seconds, Memory: 30.00 MB

The observation here is: it's measurable.

mfn

The bindings are global (ResolveInfo, SelectFields), which means even after the resolve is done and are still bound and linger around => I don't think that's a good idea. I think this is a fundamental thing: those args are effectively local data and only make sense in that specific resolver and nowhere else.

mfn · 2019-11-12T22:15:02Z

Another thing / train of thought: whilst convenient, this only affects query/mutation resolvers, not field resolvers.

But I dare not to add more complexity to the resolvers in general. But if within a field resolver one needs a dependency, one has to go back to app() again.

I wonder if it would make sense to provide more direct access to the container? E.g. via a property $laravel on the field type. Then any resolve can use it with $this->laravel->make(…).

But I'm not sure that's smart either. Containers aren't supposed to be passed around.

crissi · 2019-11-12T22:49:42Z

Another thing / train of thought: whilst convenient, this only affects query/mutation resolvers, not field resolvers.

the reason why I came up with this a final resolver will act more like a Laravel controller methods where it also possible to inject dependencies

crissi · 2019-11-12T22:52:20Z

The bindings are global (ResolveInfo, SelectFields), which means even after the resolve is done and are still bound and linger around => I don't think that's a good idea. I think this is a fundamental thing: those args are effectively local data and only make sense in that specific resolver and nowhere else.

Okay I will change that, maybe that will give some more acceptable performance.

mfn · 2019-11-13T07:17:02Z

the reason why I came up with this a final resolver will act more like a Laravel controller methods where it also possible to inject dependencies

And I too think the approach is right 👍 I don't think having the ability to auto-inject into field resolvers will do us any good in terms of performance.

The biggest result I know/work with returns (worst case) ~1000 models and queries around 20 fields I think. That wouldn't fly :-)

stevelacey · 2019-11-22T07:26:14Z

I am a big fan of this, it'd really help tidy up some queries and mutations my project has going on

stevelacey · 2019-11-22T07:55:00Z

Random: being able to inject the request likely has uses – for example, I'm currently working with phpleague oauth and throttlelogins, both expect the request object a lot.

mfn · 2019-11-22T13:42:39Z

@crissi is this ready for review?

crissi · 2019-11-22T14:15:29Z

@crissi is this ready for review?

yes

mfn

Some smaller adjustment requested, in general I'm onboard with this.

Would you also be up for updating the docs:

changelog
readme, also explaining that Closure result in SelectFields but the difference also being that the depth can be customized this way

Please ping me once done

src/Support/Field.php

tests/Support/Objects/ClassToInject.php

crissi · 2019-11-24T18:47:26Z

@mfn can you take a look again?

mfn · 2019-11-24T22:37:39Z

@crissi

I don't even need to take a look to know something's not right :-/

Can you please undo this formatting changes?

mfn · 2019-11-24T22:41:40Z

Ok, sorry, same as the other PR.

Please remove any changes you did to try to please StyleCI. It's still enabled, yes, but we actually only use php-cs fixer already. The reports from StyleCI are not required to be green for merging, so from that workflow side it's ok. It's just confusing as hell until it's completely removed.

Sorry 🤷‍♀️

crissi · 2019-11-24T23:25:17Z

Ok, sorry, same as the other PR.

Please remove any changes you did to try to please StyleCI. It's still enabled, yes, but we actually only use php-cs fixer already. The reports from StyleCI are not required to be green for merging, so from that workflow side it's ok. It's just confusing as hell until it's completely removed.

Sorry 🤷‍♀️

Did run composer fix-style, but apparently had the old phpcs config lying around so IT used that instead

crissi · 2019-11-24T23:43:28Z

Running it again correctly, but I still get the same changes...

The pipeline is an success, so does that mean the master did not have the phpcs fixes?!

mfn · 2019-11-25T13:22:45Z

Running it again correctly, but I still get the same changes...

I think to fix this should be easy as just removing/reverting all the commits you made specifically for StyleCI

crissi · 2019-11-25T19:29:50Z

@mfn it should be ready for review now

# Conflicts: # CHANGELOG.md # src/Support/Field.php

mfn

Great, thanks!

milewski · 2019-12-10T02:27:58Z

src/Support/Field.php

                throw new AuthorizationError('Unauthorized');
            }

-            return call_user_func_array($resolver, $arguments);


Why not just calling the app()->call() directly in here?

return app()->call([ $this, 'resolve' ], [ 'root' => $root, 'args' => $args, 'ctx' => $ctx, 'resolveInfo' => $arguments[3], 'selectFields' => $this->instanciateSelectFields($arguments), ]);

🤷‍♀️

You can always try making a PR!

@mfn I just notice this because I had implement this feature for auto injecting anything I pass in by myself, so on this PR I notice my solution wouldn't be needed anymore as I could just use this, however I solved it with just a few lines by calling app()->call() and this implementation is trying to do alot.... Which makes me wonder if I missed something.. Or my suggestion would be enough.. Would it work?

Would it work?

I can't say, didn't author it /cc @crissi

seems like it would bound to specific variable names instead of now where only the 3 first are required arguments and you can use what ever variable name there after as long as the Class name is correct.

mfn force-pushed the resolve_method_can_use_dependency_injection branch from ef86e0f to 7c113cd Compare November 12, 2019 21:08

mfn requested changes Nov 12, 2019

View reviewed changes

mfn requested changes Nov 23, 2019

View reviewed changes

src/Support/Field.php Outdated Show resolved Hide resolved

src/Support/Field.php Outdated Show resolved Hide resolved

src/Support/Field.php Outdated Show resolved Hide resolved

src/Support/Field.php Outdated Show resolved Hide resolved

src/Support/Field.php Outdated Show resolved Hide resolved

mfn reviewed Nov 24, 2019

View reviewed changes

src/Support/Field.php Outdated Show resolved Hide resolved

mfn reviewed Nov 24, 2019

View reviewed changes

tests/Support/Objects/ClassToInject.php Show resolved Hide resolved

crissi force-pushed the resolve_method_can_use_dependency_injection branch from 97638ef to 7e10cdb Compare November 24, 2019 18:44

crissi force-pushed the resolve_method_can_use_dependency_injection branch from 3388c8a to ba853ef Compare November 24, 2019 20:26

refactor

7b5e9a8

crissi force-pushed the resolve_method_can_use_dependency_injection branch from 2a36983 to 7b5e9a8 Compare November 25, 2019 19:27

mfn added 2 commits November 25, 2019 21:11

Merge branch 'master' into resolve_method_can_use_dependency_injection

de4c240

# Conflicts: # CHANGELOG.md # src/Support/Field.php

readme: improve grammar

7940255

mfn approved these changes Nov 25, 2019

View reviewed changes

mfn merged commit 7940255 into rebing:master Nov 25, 2019

milewski reviewed Dec 10, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resolve method can use dependency injection #520

resolve method can use dependency injection #520

crissi commented Nov 10, 2019 •

edited

Loading

mfn commented Nov 11, 2019 •

edited

Loading

mfn commented Nov 12, 2019

mfn left a comment

mfn commented Nov 12, 2019

crissi commented Nov 12, 2019

crissi commented Nov 12, 2019

mfn commented Nov 13, 2019 •

edited

Loading

stevelacey commented Nov 22, 2019

stevelacey commented Nov 22, 2019

mfn commented Nov 22, 2019

crissi commented Nov 22, 2019

mfn left a comment

crissi commented Nov 24, 2019

mfn commented Nov 24, 2019

mfn commented Nov 24, 2019

crissi commented Nov 24, 2019 •

edited

Loading

crissi commented Nov 24, 2019 •

edited

Loading

mfn commented Nov 25, 2019

crissi commented Nov 25, 2019

mfn left a comment

milewski Dec 10, 2019

mfn Dec 12, 2019

milewski Dec 13, 2019

mfn Dec 13, 2019

crissi Dec 13, 2019

resolve method can use dependency injection #520

resolve method can use dependency injection #520

Conversation

crissi commented Nov 10, 2019 • edited Loading

mfn commented Nov 11, 2019 • edited Loading

mfn commented Nov 12, 2019

mfn left a comment

Choose a reason for hiding this comment

mfn commented Nov 12, 2019

crissi commented Nov 12, 2019

crissi commented Nov 12, 2019

mfn commented Nov 13, 2019 • edited Loading

stevelacey commented Nov 22, 2019

stevelacey commented Nov 22, 2019

mfn commented Nov 22, 2019

crissi commented Nov 22, 2019

mfn left a comment

Choose a reason for hiding this comment

crissi commented Nov 24, 2019

mfn commented Nov 24, 2019

mfn commented Nov 24, 2019

crissi commented Nov 24, 2019 • edited Loading

crissi commented Nov 24, 2019 • edited Loading

mfn commented Nov 25, 2019

crissi commented Nov 25, 2019

mfn left a comment

Choose a reason for hiding this comment

milewski Dec 10, 2019

Choose a reason for hiding this comment

mfn Dec 12, 2019

Choose a reason for hiding this comment

milewski Dec 13, 2019

Choose a reason for hiding this comment

mfn Dec 13, 2019

Choose a reason for hiding this comment

crissi Dec 13, 2019

Choose a reason for hiding this comment

crissi commented Nov 10, 2019 •

edited

Loading

mfn commented Nov 11, 2019 •

edited

Loading

mfn commented Nov 13, 2019 •

edited

Loading

crissi commented Nov 24, 2019 •

edited

Loading

crissi commented Nov 24, 2019 •

edited

Loading